api: add zone, vm name params in listVmSnapshot response by shwstppr · Pull Request #4604 · apache/cloudstack

shwstppr · 2021-01-20T12:26:05Z

Description

Adds new params in listVmSnapshot API - zonename, virtualmachinename

Fixes #4603

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

UI
After change zone name and VM name are shown

CMK:
Before -

> list vmsnapshot vmsnapshotid=db83c1a7-7870-442d-8c27-d9d407a05f11
{
  "count": 1,
  "vmSnapshot": [
    {
      "account": "admin",
      "created": "2021-01-20T17:34:35+0530",
      "current": true,
      "displayname": "vm-snap",
      "domain": "ROOT",
      "domainid": "52611b65-5b16-11eb-b67a-645d8651f45a",
      "hypervisor": "Simulator",
      "id": "db83c1a7-7870-442d-8c27-d9d407a05f11",
      "name": "i-2-3-VM_VS_20210120120435",
      "state": "Ready",
      "tags": [],
      "type": "Disk",
      "virtualmachineid": "3a07d2d4-18ad-4ef4-ac97-7937ba629069",
      "zoneid": "5918c672-47aa-4425-9ff9-d71d79d2b304",
    }
  ]
}

After-

> list vmsnapshot vmsnapshotid=db83c1a7-7870-442d-8c27-d9d407a05f11
{
  "count": 1,
  "vmSnapshot": [
    {
      "account": "admin",
      "created": "2021-01-20T17:34:35+0530",
      "current": true,
      "displayname": "vm-snap",
      "domain": "ROOT",
      "domainid": "52611b65-5b16-11eb-b67a-645d8651f45a",
      "hypervisor": "Simulator",
      "id": "db83c1a7-7870-442d-8c27-d9d407a05f11",
      "name": "i-2-3-VM_VS_20210120120435",
      "state": "Ready",
      "tags": [],
      "type": "Disk",
      "virtualmachineid": "3a07d2d4-18ad-4ef4-ac97-7937ba629069",
      "virtualmachinename": "test-vm",
      "zoneid": "5918c672-47aa-4425-9ff9-d71d79d2b304",
      "zonename": "Sandbox-simulator"
    }
  ]
}

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

blueorangutan · 2021-01-20T13:26:26Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2555

harikrishna-patnala

LGTM

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

harikrishna-patnala · 2021-01-21T06:22:39Z

server/src/main/java/com/cloud/api/ApiResponseHelper.java

        if (vm != null) {
-            vmSnapshotResponse.setVirtualMachineid(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineId(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineName(vm.getHostName());


I think display_name should be first choice, if it is null then we can go with host_name.

okay @harikrishna-patnala

Thanks @shwstppr, looks good to me now

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

sureshanaparti · 2021-01-21T07:46:57Z

@shwstppr I do understand that it is good to display name(s) of resources/entities in the UI. But, I think, to change the respective API responses to include the name(s) of the required resources/entities (to show in the UI) may not be the right thing. The UUIDs can be further used with the list API(s) to get the required details (name, zone, etc) to fill the UI as desired.

If we continue this exercise to show name(s) in the UI, finally we end up adding name(s) to most of the responses.

I know UI has to attempt multiple API calls, but API responses should primarly return UUIDs. For names and other details, make use of list API calls.

blueorangutan · 2021-01-21T08:52:36Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2562

yadvr · 2021-01-21T12:43:03Z

@shwstppr is this for 4.15 or master, pl change base branch or milestone

shwstppr · 2021-01-21T13:02:30Z

@rhtyd sorry for the error. Changed the base branch

@blueorangutan package

blueorangutan · 2021-01-21T13:03:40Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

shwstppr · 2021-01-21T13:07:38Z

@shwstppr I do understand that it is good to display name(s) of resources/entities in the UI. But, I think, to change the respective API responses to include the name(s) of the required resources/entities (to show in the UI) may not be the right thing. The UUIDs can be further used with the list API(s) to get the required details (name, zone, etc) to fill the UI as desired.

If we continue this exercise to show name(s) in the UI, finally we end up adding name(s) to most of the responses.

I know UI has to attempt multiple API calls, but API responses should primarly return UUIDs. For names and other details, make use of list API calls.

@sureshanaparti I may agree with you on that partly but changes aren't causing any additional db query, plus we have similar parameters in other api responses so maybe this can be allowed ;)
Calling list api just for names in UI will be an overkill

blueorangutan · 2021-01-21T16:41:11Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2568

harikrishna-patnala · 2021-01-22T05:00:06Z

@shwstppr I do understand that it is good to display name(s) of resources/entities in the UI. But, I think, to change the respective API responses to include the name(s) of the required resources/entities (to show in the UI) may not be the right thing. The UUIDs can be further used with the list API(s) to get the required details (name, zone, etc) to fill the UI as desired.
If we continue this exercise to show name(s) in the UI, finally we end up adding name(s) to most of the responses.
I know UI has to attempt multiple API calls, but API responses should primarly return UUIDs. For names and other details, make use of list API calls.

@sureshanaparti I may agree with you on that partly but changes aren't causing any additional db query, plus we have similar parameters in other api responses so maybe this can be allowed ;)
Calling list api just for names in UI will be an overkill

Agree with you @shwstppr

shwstppr · 2021-01-22T07:21:11Z

@blueorangutan test

blueorangutan · 2021-01-22T07:21:49Z

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

sureshanaparti · 2021-01-22T10:13:28Z

@shwstppr I do understand that it is good to display name(s) of resources/entities in the UI. But, I think, to change the respective API responses to include the name(s) of the required resources/entities (to show in the UI) may not be the right thing. The UUIDs can be further used with the list API(s) to get the required details (name, zone, etc) to fill the UI as desired.
If we continue this exercise to show name(s) in the UI, finally we end up adding name(s) to most of the responses.
I know UI has to attempt multiple API calls, but API responses should primarly return UUIDs. For names and other details, make use of list API calls.

@sureshanaparti I may agree with you on that partly but changes aren't causing any additional db query, plus we have similar parameters in other api responses so maybe this can be allowed
Calling list api just for names in UI will be an overkill

@shwstppr So, do you mean it is the right approach to consider the API response object parameters for UI display? Few recent PRs below, with such changes where the response parameters are mainly updated for UI display only, and I'm sure this will repeat to replace most of the UUID displayed in the UI for the names. :-)

#4483 - updated listNetworks API response for VPC Name
#4275 - updated listVmSnapshot API response for Hypervisor
#4073 - updated listPublicIpAddress API response for Network Name

sureshanaparti · 2021-01-22T10:25:33Z

@shwstppr May be some generic approach for the API response objects, to tag both UUID and Name (wherever UUID is returned) might work for all the APIs, and the names can be used appropriately in the UI when required.

blueorangutan · 2021-01-22T17:34:46Z

Trillian test result (tid-3408)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35127 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4604-t3408-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_07_deploy_kubernetes_ha_cluster	`Failure`	3619.44	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	64.07	test_kubernetes_clusters.py

harikrishna-patnala · 2021-01-25T03:39:23Z

@shwstppr May be some generic approach for the API response objects, to tag both UUID and Name (wherever UUID is returned) might work for all the APIs, and the names can be used appropriately in the UI when required.

@sureshanaparti I get your proposal, but that I think it needs significant amount of changes as Name is not part of common interfaces (which I think is required for a generic approach) and it is defined as different names for each resource.

May be an another improvement ticket would help this PR to get continued !

yadvr · 2021-01-25T08:25:11Z

@blueorangutan ui

yadvr · 2021-01-25T08:25:57Z

@blueorangutan ui

blueorangutan · 2021-01-25T08:26:07Z

@rhtyd a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

yadvr · 2021-01-25T12:41:52Z

@blueorangutan ui

blueorangutan · 2021-01-25T12:42:58Z

@rhtyd a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2021-01-25T12:57:28Z

UI build: ✔️
Live QA URL: http://primate-qa.cloudstack.cloud:8080/client/pr/4604 (JID-3829)

yadvr · 2021-02-12T07:14:03Z

server/src/main/java/com/cloud/api/ApiResponseHelper.java

-            vmSnapshotResponse.setVirtualMachineid(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineId(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineName(Strings.isNullOrEmpty(vm.getDisplayName()) ? vm.getHostName() : vm.getDisplayName());
+            vmSnapshotResponse.setVirtualMachineName(vm.getHostName());


Why is setVirtualMachineName() added twice @shwstppr ?

@rhtyd that was by mistake in my previous commit 🤦 . Fixed it

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr · 2021-02-12T10:14:13Z

@blueorangutan package

blueorangutan · 2021-02-12T10:15:15Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-02-12T11:24:12Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2686

yadvr · 2021-02-18T12:52:27Z

server/src/main/java/com/cloud/api/ApiResponseHelper.java

        if (vm != null) {
-            vmSnapshotResponse.setVirtualMachineid(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineId(vm.getUuid());
+            vmSnapshotResponse.setVirtualMachineName(Strings.isNullOrEmpty(vm.getDisplayName()) ? vm.getHostName() : vm.getDisplayName());


LGTM - we need one check if for normal user/account it leaks the internal hostname (is hostname the i-x-y-VM?)

checked hostname would return name from the Db table, lgtm

api: add zone, vm name params in listVmSnaphots response

3bc06ba

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

shwstppr added component:api component:UI labels Jan 20, 2021

shwstppr added this to the 4.15.1.0 milestone Jan 20, 2021

shwstppr self-assigned this Jan 20, 2021

shwstppr changed the title ~~api: add zone, vm name params in listVmSnaphots response~~ api: add zone, vm name params in listVmSnaphot response Jan 20, 2021

shwstppr changed the title ~~api: add zone, vm name params in listVmSnaphot response~~ api: add zone, vm name params in listVmSnapshot response Jan 20, 2021

harikrishna-patnala approved these changes Jan 21, 2021

View reviewed changes

display name can be empty

7ae3b78

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

harikrishna-patnala reviewed Jan 21, 2021

View reviewed changes

prefer vm display name over hostname

bffd70a

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

apache deleted a comment from blueorangutan Jan 21, 2021

shwstppr changed the base branch from master to 4.15 January 21, 2021 12:59

apache deleted a comment from blueorangutan Jan 21, 2021

shwstppr requested review from Pearl1594 and davidjumani January 26, 2021 06:36

yadvr reviewed Feb 12, 2021

View reviewed changes

fix

5efc277

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>

yadvr reviewed Feb 18, 2021

View reviewed changes

yadvr approved these changes Feb 19, 2021

View reviewed changes

yadvr closed this Feb 19, 2021

yadvr reopened this Feb 19, 2021

yadvr merged commit f489439 into apache:4.15 Feb 19, 2021

shwstppr mentioned this pull request Feb 23, 2021

uuids instead of display names are shown in detail view of VM snapshot #4603

Closed

Conversation

shwstppr commented Jan 20, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

blueorangutan commented Jan 20, 2021

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

harikrishna-patnala Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Jan 21, 2021

Uh oh!

yadvr commented Jan 21, 2021

Uh oh!

shwstppr commented Jan 21, 2021

Uh oh!

blueorangutan commented Jan 21, 2021

Uh oh!

shwstppr commented Jan 21, 2021

Uh oh!

blueorangutan commented Jan 21, 2021

Uh oh!

harikrishna-patnala commented Jan 22, 2021

Uh oh!

shwstppr commented Jan 22, 2021

Uh oh!

blueorangutan commented Jan 22, 2021

Uh oh!

sureshanaparti commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sureshanaparti commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented Jan 22, 2021

Uh oh!

harikrishna-patnala commented Jan 25, 2021

Uh oh!

yadvr commented Jan 25, 2021

Uh oh!

yadvr commented Jan 25, 2021

Uh oh!

blueorangutan commented Jan 25, 2021

Uh oh!

yadvr commented Jan 25, 2021

Uh oh!

blueorangutan commented Jan 25, 2021

Uh oh!

blueorangutan commented Jan 25, 2021

Uh oh!

yadvr Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

shwstppr commented Feb 12, 2021

Uh oh!

blueorangutan commented Feb 12, 2021

Uh oh!

blueorangutan commented Feb 12, 2021

Uh oh!

yadvr Feb 18, 2021

sureshanaparti commented Jan 21, 2021 •

edited

Loading

sureshanaparti commented Jan 22, 2021 •

edited

Loading

sureshanaparti commented Jan 22, 2021 •

edited

Loading